Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

dev,genbzl: add support for generating syntax diagrams #84451

Merged
merged 1 commit into from
Jul 18, 2022
Merged

dev,genbzl: add support for generating syntax diagrams #84451

merged 1 commit into from
Jul 18, 2022

Conversation

ajwerner
Copy link
Contributor

Fixes #84443.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner ajwerner mentioned this pull request Jul 14, 2022
Closed
@ajwerner ajwerner marked this pull request as ready for review July 15, 2022 03:30
@ajwerner ajwerner requested a review from a team as a code owner July 15, 2022 03:30
@ajwerner ajwerner requested a review from nickvigilante July 15, 2022 03:30
@nickvigilante
Copy link
Contributor

nickvigilante commented Jul 15, 2022 •
edited
Loading

@ajwerner I tested this locally and this accomplishes part of it and is already super helpful, but we also need to generate the raw BNF files so the docgen CI job can populate the generated-diagrams repo. I can try that once I get started in Dev Inf. Otherwise, great work and LGTM!

Edit: Although, I suppose we can just edit the CI job to copy the HTML files only and do away with the extra step of the BNF. I'll have to talk to the docs team about that.

rickystewart
rickystewart approved these changes Jul 15, 2022
View reviewed changes
dev Outdated
@@ -3,7 +3,7 @@
set -euo pipefail

# Bump this counter to force rebuilding `dev` on all machines.
DEV_VERSION=41
DEV_VERSION=42
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

42 is the current version, you probably want to rebase and bump this again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ajwerner ajwerner force-pushed the ajwerner/generate-diagrams branch from ff3b4b5 to 2d9a886 Compare July 18, 2022 14:45
@ajwerner
Copy link
Contributor Author

@ajwerner I tested this locally and this accomplishes part of it and is already super helpful, but we also need to generate the raw BNF files so the docgen CI job can populate the generated-diagrams repo. I can try that once I get started in Dev Inf. Otherwise, great work and LGTM!

It was trivial to add support for generating the bnf files. I've updated the change to also include a ./dev generate bnf subcommand.

nickvigilante
nickvigilante approved these changes Jul 18, 2022
View reviewed changes
Copy link
Contributor

@nickvigilante nickvigilante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ajwerner You're awesome.

@ajwerner
Copy link
Contributor Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 18, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jul 18, 2022

Build succeeded:

@craig craig bot merged commit 7191d81 into cockroachdb:master Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvigilante nickvigilante nickvigilante approved these changes

@rickystewart rickystewart rickystewart approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move SQL grammar diagram generation from Makefile to Bazel
4 participants
@ajwerner @cockroach-teamcity @nickvigilante @rickystewart